Fix .NET Version Parsing for FindPath API #2000
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The old comparison used string comparison which is fine for versions of dotnet until dotnet 10. '10.0' < '9.0'. 😂 This fixes that. I suppose it's a bit clunky and we could dedupe / share the compare logic but it's not being used elsewhere in this same form, so I don't plan to do that right now. There are also places that depend on the output versions being strings, so that's a problem to change that now too. It probably should have been a class from the beginning but unfortunately that just isn't how it was initially done, which is understandable because the scope at that time was much more limited.
nagilson
commented
Oct 23, 2024
vscode-dotnet-runtime-extension/src/test/functional/DotnetCoreAcquisitionExtension.test.ts
Outdated
Show resolved
Hide resolved
smitpatel
reviewed
Oct 25, 2024
| @@ -136,22 +136,38 @@ Please set the PATH to a dotnet host that matches the architecture ${requirement | |||
| return os.platform() === 'win32' ? (await this.executor!.tryFindWorkingCommand([CommandExecutor.makeCommand('chcp', ['65001'])])) !== null : false; | |||
| } | |||
|
|
|||
| private stringVersionMeetsRequirement(foundVersion : string, requiredVersion : string, requirement : DotnetVersionSpecRequirement) : boolean | |||
| private stringVersionMeetsRequirement(availableVersion : string, requestedVersion : string, requirement : DotnetVersionSpecRequirement) : boolean | |||
| { | |||
There was a problem hiding this comment.
That's a good suggestion, semver is a good option. I hadn't considered that, thank you. I wish we were using semver in the code a lot earlier in many places.
Going forward I will use that. I do want to get this merged quickly though, so I'm not sure about making that change now considering this is already written and tested 🤔
There was a problem hiding this comment.
Can you file an issue to use semver rather than parsing versions ourselves?
smitpatel
approved these changes
Oct 25, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The old comparison used string comparison which is fine for versions of dotnet until dotnet 10. '10.0' < '9.0'. 😂
This fixes that. I suppose it's a bit clunky and we could dedupe / share the compare logic but it's not being used elsewhere in this same form, so I don't plan to do that right now. There are also places that depend on the output versions being strings, so that's a problem to change that now too. It probably should have been a class from the beginning but unfortunately that just isn't how it was initially done, which is understandable because the scope at that time was much more limited.